url: disallow invalid IPv4 in IPv6 parser #12315
Conversation
nodejs-github-bot
added
c++
src/node_url.cc
Outdated
| while (ch != kEOL) { | ||
| value = 0xffffffff; | ||
| if (numbers_seen > 0) { | ||
| if (ch == '.' && 4 > numbers_seen) { |
There was a problem hiding this comment.
Maybe numbers_seen < 4 is a bit more intuitive than 4 > numbers_seen?
There was a problem hiding this comment.
Sure thing. I've updated it :)
watilde
force-pushed
the
feature/ipv4-in-ipv6
branch
from
37de626 to
521926a
Compare
src/node_url.cc
Outdated
| pointer++; | ||
| ch = pointer < end ? pointer[0] : kEOL; | ||
| if (value > 255) | ||
| goto end; |
There was a problem hiding this comment.
Oh, I was just reading the spec from the top to the bottom. The order comes from it, but yeah it's better to not touch for the performance. I will update it :)
src/node_url.cc
Outdated
| ch = pointer < end ? pointer[0] : kEOL; | ||
| } | ||
| if (dots == 3 && ch != kEOL) | ||
| if (ch == kEOL && numbers_seen != 4) |
There was a problem hiding this comment.
This is covered by the if (numbers_seen > 0) { check at the start of the loop, isn't it?
There was a problem hiding this comment.
I think it could happen if the numbers_seen is increased in the loop in the loop that the top loop can't detect: https://github.com/watilde/node/blob/521926ae2f502759c5fc752c82a2661a3dbf419e/src/node_url.cc#L179
There was a problem hiding this comment.
I think I see what you mean but in that case it can be moved to right after the loop, right? And the ch == kEOL clause can be dropped because that's implied by while (ch != kEOL).
There was a problem hiding this comment.
Oh you're right! I just got what you meant of right after the loop. I will update and let's wait for the spec update at whatwg/url#292. Thanks :)
There was a problem hiding this comment.
The spec was updated at whatwg/url#292.
watilde
force-pushed
the
feature/ipv4-in-ipv6
branch
from
521926a to
a5786ac
Compare
|
Landed in 1b99d8f. Thanks! |
Fixes: #10655 PR-URL: #12315 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Timothy Gu <[email protected]>
Fixes: #10655.
Checklist
make -j4 testAffected core subsystem(s)
url